Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: add idmap and ridmap mount options #1222

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Aug 22, 2023

Adding new fields for MOUNT_ATTR_IDMAP had the flaw that users
specifying these fields with older runtimes would result in the fields
being ignored and incorrect mounts being configured. In addition, there
is no text in the specification indicating whether MOUNT_ATTR_IDMAP
should be applied with AT_RECURSIVE (which matters for rbind idmapped
mounts).

In retrospect, the addition of the fields should've included new (dummy)
mount options that would cause errors on older runtimes. Unfortunately,
we have had a runtime-spec release since then so we cannot MUST these
new mount options, but we can SHOULD them.

Fixes #1216
Resolves the main issue motivating the urgency of #1219 (though that feature is still something we should have).

Fixes: 9d1130d ("IDMapping field for mount point")
Signed-off-by: Aleksa Sarai [email protected]

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if this indeed causes the runtimes to fail when the option is not recognized.

config.md Show resolved Hide resolved
Adding new fields for MOUNT_ATTR_IDMAP had the flaw that users
specifying these fields with older runtimes would result in the fields
being ignored and incorrect mounts being configured. In addition, there
is no text in the specification indicating whether MOUNT_ATTR_IDMAP
should be applied with AT_RECURSIVE (which matters for rbind idmapped
mounts).

In retrospect, the addition of the fields should've included new (dummy)
mount options that would cause errors on older runtimes. Unfortunately,
we have had a runtime-spec release since then so we cannot MUST these
new mount options, but we can SHOULD them.

Fixes: 9d1130d ("IDMapping field for mount point")
Signed-off-by: Aleksa Sarai <[email protected]>
The idmapped mounts sections do not make any reference to how the
mapping should be implemented. Add a reference to MOUNT_ATTR_IDMAP since
that is what runtimes are expected to use.

Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar
Copy link
Member Author

cyphar commented Aug 22, 2023

@rata The error message is fairly ugly, but the mount will fail because no filesystem supports the flag "idmap" nor "ridmap".

... Oh actually, now that I think about it, I think this flag will be ignored for bind-mounts due to how they are implemented in runc (extra mount options for bind-mounts are ignored because that's what the kernel does). Damn. This is going to be ugly... I will need to think about this tomorrow (at least in the case of runc, I suspect we should actually error out if the mount has extra fs options that the kernel will ignore).

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if this does old runtimes fail (runc and crun at least) and post the error it shows here, so we can easily know the impact of this change.

@cyphar
Copy link
Member Author

cyphar commented Aug 22, 2023

This is the error you get if you add it to a regular mount (for runc):

ERRO[0000] runc run failed: unable to start container process: error during container init: error mounting "cgroup" to rootfs at "/sys/fs/cgroup": mount src=cgroup, dst=/sys/fs/cgroup, dstFd=/proc/self/fd/8, flags=0x20000f, data=idmap: invalid argument

But for bind-mounts there's no error.

@rata
Copy link
Member

rata commented Aug 22, 2023

Then this is not useful as-is, at least :(

@cyphar
Copy link
Member Author

cyphar commented Aug 22, 2023

I think we need ridmap anyway (because of #1216). I've opened a PR against runc to detect this properly (opencontainers/runc#3990):

ERRO[0000] runc run failed: invalid mount &{Source:/tmp Destination:foo Device:bind Flags:2101263 ClearedFlags:0 PropagationFlags:[] Data:idmap Relabel: RecAttr:<nil> Extensions:0 UIDMappings:[] GIDMappings:[]}: bind mounts cannot have any filesystem-specific options applied

Unfortunately this doesn't help with old versions of runc (though the same is true for #1219 because we need to update runtimes to say whether they support idmapped mounts), and I suspect other runtimes have the same behaviour because it's easy to miss this.

@giuseppe @utam0k How do crun and youki handle data flags being passed to bind-mounts? Do you return errors or are the flags silently ignored? (Try adding idmap to a bind-mount options list.)

@AkihiroSuda
Copy link
Member

Adding mount options which do NOP by themselves looks clumsy.

Why not just use the mountExtensions struct?

Maybe this could be an object like:

"mountExtensions": {
  "idmap": {
    "modes": ["sameAsUserns", "arbitrary"],
    "filesystemTypes": ["ext4", "tmpfs", "foobarfs"],
    "supportsBind": true
  }
}

wdyt?

Also we need to figure out how to deal with recursive mounts -- see #1216. If we add a separate flag for this, we will need to add an entry to this struct.

Originally posted by @cyphar in #1219 (comment)

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer mountExtensions struct

#1222 (comment)

@cyphar
Copy link
Member Author

cyphar commented Aug 23, 2023

@AkihiroSuda The features proposal doesn't help with:

  • Indicating whether the mapping should be applied with AT_RECURSIVE. This requires some kind of flag on the mount.
  • As far as I can tell, we don't have any other runtime-spec features that require you to check the features list in order to avoid a setting being silently ignored. The annotations stuff you're proposing is in the same ballpark but I think there's no other way to deal with annotations -- for mounts we have a specific way of specifying what features we need.

Also, the flags aren't NOP flags -- they indicate you need to do a certain mount operation (just like "rro" and the other recursive mount settings).

That being said, I think we need the features addition as well.

@rata
Copy link
Member

rata commented Aug 23, 2023

@cyphar

  • As far as I can tell, we don't have any other runtime-spec features that require you to check the features list in order to avoid a setting being silently ignored

I guess there is some other. By design, runtimes MUST ignore unknown fields. Probably in practice most of the stuff is supported for years, so not a real issue, but some example probably exists (probably also not with such awful consequences as this one).

@cyphar
Copy link
Member Author

cyphar commented Aug 23, 2023

Hmm, now that I think about it some more, that's a good point @rata...

FWIW, I'm starting to come to the view that the way we planned for extensibility in the spec was a bit misguided -- runtimes do not reject configurations from specification versions they do not support, which is behaviour that just begging to cause a security issue in the future. The purpose of the "runtimes MUST ignore unknown fields" text was to allow for vendor extensions (I think this was also motivated by the fact that the Go stdlib JSON parser didn't have a way to block unknown fields when we first started working on the spec -- DisallowUnknownFields was added in Go 1.10 in 2018 -- which was after the runtime-spec 1.0 release in 2017). While there is an argument for allowing vendor extensions to the image-spec that tools should ignore if unsupported, I don't think the argument makes as much sense for the runtime-spec (and the image-spec has had their own issues with this when dealing with registries...). Obviously it's far too late to change any of this now...

For this particular feature, we should've caught this issue when reviewing the idmapped mounts additions to the spec.

All of that being said, we need ridmap to resolve #1216 and if we have ridmap I don't see any downside to having idmap as well.

@rata
Copy link
Member

rata commented Aug 23, 2023

@cyphar I fully agree with that. I did a patch to runc, to fail on unknown fields, but never opened a PR as I found that note about extensibility. I completely share that that note creates more issues than it solves.

There might be some "escapes" (besides going for v2.0.0, that I don't think is a good idea), like adding a failOnUnknownFields: true somewhere in the config.json, that high-level runtimes could set (maybe also even query support for that using the features thing) and some years in the future, when all used OCI runtimes support that flag, then that would be universal. At that point, maybe high-level runtimes can just fail if that feature is not implemented, and not even worry about providing a fall-back to be safe in the face of OCI runtime unknown features.

That is a longer conversation and I'm not 100% sure what my thoughts are about it now.

@giuseppe
Copy link
Member

@giuseppe @utam0k How do crun and youki handle data flags being passed to bind-mounts? Do you return errors or are the flags silently ignored? (Try adding idmap to a bind-mount options list.)

crun for bind mounts would just ignore a flag it doesn't know about.

crun already supports idmap with a slightly different meaning though. If it is specified then it uses the same mappings used for the container user namespace. I was playing with it before uidMappings/gidMappings were added to the OCI specs: containers/crun#780

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But the PR description is out of date (it doesn't mitigate the urgency of the other feature, this doesn't fail on old runtimes as we first thought).

@utam0k
Copy link
Member

utam0k commented Aug 25, 2023

@giuseppe @utam0k How do crun and youki handle data flags being passed to bind-mounts? Do you return errors or are the flags silently ignored? (Try adding idmap to a bind-mount options list.)

Youki ignores a flag that we don't know about.
https://github.com/containers/youki/blob/f2d95ca41c1d2c83af2f87e28b31c77c835dfad6/crates/libcontainer/src/rootfs/utils.rs#L80-L121

@utam0k utam0k mentioned this pull request Aug 25, 2023
2 tasks
@utam0k utam0k merged commit 305605a into opencontainers:main Aug 25, 2023
3 checks passed
@AkihiroSuda
Copy link
Member

crun already supports idmap with a slightly different meaning though.

This PR had to be updated to avoid conflict with crun?

@utam0k
Copy link
Member

utam0k commented Aug 25, 2023

Sorry for missing it. I thought it was fine, especially since there was no objection, but I should have checked.

@giuseppe The name of the mount options in crun seem to overlap, is that OK?

If the idmap option is specified then the mount is ID mapped using the container target user namespace. This is an experimental feature and can change at any time without notice.
https://github.com/giuseppe/crun/blob/827b8731899d4febea3f27907bcde5e6065bb65b/crun.1.md#idmap-mount-options

@cyphar
Copy link
Member Author

cyphar commented Aug 27, 2023

We will need to change the MUST language to allow crun's behaviour. In fact, maybe we should make crun's behaviour the standard behaviour if the mount doesn't have a mapping specified (it seems reasonable to me to default to the container mapping). I'm on vacation for the next two weeks, if someone else wants to pick this up in the meantime I'd appreciate it 😅

@AkihiroSuda
Copy link
Member

@giuseppe WDYT? 🙏

@giuseppe
Copy link
Member

giuseppe commented Sep 5, 2023

what would be the best wording to allow the crun behaviour? Just dropping the following sentence?

If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.

@giuseppe
Copy link
Member

giuseppe commented Sep 5, 2023

opened a PR, we can discuss there how it should be: #1224

@cyphar cyphar deleted the idmap-flag branch October 23, 2023 22:49
@utam0k utam0k mentioned this pull request Jan 5, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idmapped mounts: should they be applied recursively?
5 participants